Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

dsl: Overhaul custom fd weights #1644

Closed
wants to merge 3 commits into from
Closed

dsl: Overhaul custom fd weights #1644

wants to merge 3 commits into from

Conversation

rhodrin
Copy link
Contributor

@rhodrin rhodrin commented Mar 24, 2021

Work in progress. TBC.

Aside from the actual implementation, we'll also need to discuss legacy support at some stage.

@rhodrin rhodrin added WIP Still work in progress better code API api (symbolics, types, ...) labels Mar 24, 2021
@codecov
Copy link

codecov bot commented Mar 24, 2021 •
edited
Loading

Codecov Report

Merging #1644 (e428923) into master (cde7a8e) will decrease coverage by 18.60%.
The diff coverage is 59.09%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1644       +/-   ##
===========================================
- Coverage   88.88%   70.28%   -18.61%     
===========================================
  Files         210      162       -48     
  Lines       33634    21267    -12367     
  Branches     4391     3493      -898     
===========================================
- Hits        29896    14947    -14949     
- Misses       3245     5720     +2475     
- Partials      493      600      +107
Impacted Files Coverage Δ
devito/finite_differences/__init__.py 100.00% <ø> (ø)
devito/finite_differences/tools.py 79.03% <ø> (-11.20%) ⬇️
devito/types/dense.py 75.41% <ø> (-17.96%) ⬇️
devito/finite_differences/finite_difference.py 72.91% <36.36%> (-23.09%) ⬇️
devito/finite_differences/derivative.py 72.02% <80.00%> (-14.40%) ⬇️
devito/types/equation.py 85.71% <100.00%> (-12.99%) ⬇️
devito/core/autotuning.py 13.18% <0.00%> (-80.00%) ⬇️
devito/passes/clusters/buffering.py 20.32% <0.00%> (-75.09%) ⬇️
devito/data/utils.py 21.83% <0.00%> (-70.75%) ⬇️
devito/builtins/initializers.py 24.26% <0.00%> (-67.65%) ⬇️
... and 144 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cde7a8e...e428923. Read the comment docs.

FabioLuporini
FabioLuporini reviewed Mar 25, 2021
View reviewed changes
Copy link
Contributor

@FabioLuporini FabioLuporini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking really really nice :)

devito/finite_differences/derivative.py
if self.ndims == 1:
_fd_order = fd_order or self._fd_order
_side = side or self._side
new_x0 = {self.dims[0]: x0} if x0 is not None else self.x0
return self._new_from_self(fd_order=_fd_order, side=_side, x0=new_x0)
new_coeffs = coeffs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coeffs or self._coeffs ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to check - something wasn't working last night so wrote it in this way for now but quite a few things in there need checking and fixing still.

@FabioLuporini
Copy link
Contributor

FabioLuporini commented Mar 25, 2021 •
edited
Loading

@rhodrin

Aside from the actual implementation, we'll also need to discuss legacy support at some stage.

First thought: drop and burn release

Second thought: provide a mini (mini) tutorial explaining how to transition from old to new

@mloubout
Copy link
Contributor

RIght what I had in mind. The it'll be trivial to extend it to allowing (ind, weight) pairs as coeffs for people who really want custom stencils in general (like 3x3 kernel for first derivative)

For legacy, not completely sure yes to be discussed.

@rhodrin
Copy link
Contributor Author

rhodrin commented Mar 25, 2021 •
edited
Loading

First thought: drop and burn release

Second thought: provide a mini (mini) tutorial explaining how to transition from old to new

Sounds good to me. We could add a new tutorial in the userapi folder which also explains the transformation and we'll also need to update the DRP tutorial of course.

@rhodrin
Copy link
Contributor Author

rhodrin commented Mar 25, 2021 •
edited
Loading

RIght what I had in mind. The it'll be trivial to extend it to allowing (ind, weight) pairs as coeffs for people who really want > custom stencils in general (like 3x3 kernel for first derivative)

For legacy, not completely sure yes to be discussed.

Yeah, indeed. Will implement that functionality. It's just some minor editing here and there.

Re. legacy support, we could for the time being add a decorator to Eq that based on the equation + presence of a coefficients object just reassembles the equation in the new form. It's not pretty, but should be fairly simple to implement and then cut out when we deprecate.

@FabioLuporini FabioLuporini changed the title Overhaul custom fd weights dsl: Overhaul custom fd weights Apr 2, 2021
@FabioLuporini FabioLuporini mentioned this pull request Apr 21, 2021
Closed
@rhodrin rhodrin force-pushed the overhaul_custom_coeffs branch from dd36e5f to e428923 Compare July 8, 2021 15:05
@mloubout mloubout mentioned this pull request Jul 31, 2024
Merged
@mloubout
Copy link
Contributor

Done in #2434 closing (easier than rebase that old one)

@mloubout mloubout closed this Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FabioLuporini FabioLuporini FabioLuporini left review comments

Assignees
No one assigned
Labels
API api (symbolics, types, ...) WIP Still work in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rhodrin @FabioLuporini @mloubout